-
-
Notifications
You must be signed in to change notification settings - Fork 18.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
REF: algos_take_helper de-nest templating #30413
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea seems strange so worth refactoring; can you also confirm no new build warnings?
@@ -184,7 +107,29 @@ def take_1d_{{name}}_{{dest}}(ndarray[{{c_type_in}}, ndim=1] values, | |||
# We cannot use the memoryview version on readonly-buffers due to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still valid? Seems like making these functions cpdef
would solve some of the problems I guess this was trying to solve originally
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking the blame, i think you're right that this is outdated, but not that cpdef would help. i think using the const
modifier would handle this in most of the relevant cases, but we'll still need to do some gymnastics with object[:]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so this was done IIRC to guard again the readonly array issue. if that is fixed (later cython I think)? then this is fine. any perf impact?
yah, that's the crux of what @WillAyd mentioned. do we want to move to the more modern usage in this PR or next pass? |
ok here, any perf impact? |
Updated to use the modern pattern. an asv run with |
No evident perf impact.
|
Having two layers of templating here is really weird, this unravels one layer so this file is organized like our other tempita files. That makes future moves towards fused types and/or conditional nogil feasible.